Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cloudfront): trim autogenerated cache policy name #18953

Merged
merged 6 commits into from
Feb 22, 2022

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Feb 12, 2022

Closes #18918


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Feb 12, 2022

@github-actions github-actions bot added the @aws-cdk/aws-cloudfront Related to Amazon CloudFront label Feb 12, 2022
@robertd
Copy link
Contributor Author

robertd commented Feb 12, 2022

@RigoIce Thanks for figuring out the main cause given the generic cloudformation error. 👍

@ghost
Copy link

ghost commented Feb 12, 2022

Hi @robertd. I just wanted to jump into a discussion about that PR.

I already had a look into the code and found out that a fix just slicing the name would probably effect the appended region. Under some circumstances we could again have same names in different regions for the policies. The region suffix was introduced in #13737 closing #13629.

I thought more about slicing the expression Names.uniqueId(this). This would probably cut of the hash what would be less problematic, I think. Also, there shouldn't be any backward problems as this only effects non-working policies with name lengths above 128 and still keeps the uniqueness across the regions.

However, that seemed not to cover all facets for me as well because the hash could serve as a unique identifier within a region. I wasn't convinced whether my proposed slicing approach could cause problems with uniqueness in one region, i.e. multiple policies.

So, this is the reason I haven't proposed a PR on my own as I see a possible solution with a backward compatible optional parameter for uniqueId() limiting the part before the hash. But this is going all the way down to CDK core just for handling exceptional cases.

Maybe my considerations are too complicated or maybe the idea could also help in other cases where a shorter ID is required.

Happy to discuss and support.

@robertd
Copy link
Contributor Author

robertd commented Feb 15, 2022

@RigoIce Makes sense... Although, slicing the uniqueId will have to take in consideration region concatenation... and that becomes somewhat complicated once you get into longer regions (i.e. ap-southeast-3).

I'm not 100% sure what would be the best scenario here... slice uniqueId down to 110 characters and then append region? ... just to be on the safe side?

@comcalvi Any thoughts?

@comcalvi
Copy link
Contributor

comcalvi commented Feb 21, 2022

I agree with that @robertd, we need to maintain the cross-region safety introduced in #13737. I see no issue with slicing it down to 110 characters and then appending the region.

@robertd
Copy link
Contributor Author

robertd commented Feb 21, 2022

Thanks for chiming in @comcalvi. I’ll update the PR.

@robertd
Copy link
Contributor Author

robertd commented Feb 21, 2022

@comcalvi ready for your review. Thanks.

comcalvi
comcalvi previously approved these changes Feb 21, 2022
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, this is great!

@mergify
Copy link
Contributor

mergify bot commented Feb 21, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed comcalvi’s stale review February 21, 2022 23:41

Pull request has been modified.

@robertd
Copy link
Contributor Author

robertd commented Feb 21, 2022

@comcalvi I had to merge in the latest master because build failed for some weird reason. Please reapprove the PR again. TIA.

@comcalvi comcalvi self-requested a review February 21, 2022 23:57
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ff6cb52
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 22, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit c7394c9 into aws:master Feb 22, 2022
@robertd robertd deleted the robertd/cloudfront branch February 22, 2022 00:23
robertd added a commit to robertd/aws-cdk that referenced this pull request Feb 22, 2022
Closes aws#18918

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cloudfront): Name length of cache policy
3 participants